Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement IStorageProvider OpenFile/OpenFolder/SaveFile and MessageBox dialogs #153

Merged
merged 71 commits into from
Nov 30, 2024

Conversation

tomlm
Copy link
Collaborator

@tomlm tomlm commented Nov 25, 2024

This is fix for #149
Features:

  • Implements IStorageProvider/IStorageFile/IStorageFolder
  • Implements FileOpen/FolderOpen/FileSave dialogs

File open

FileOpen

File Open with multiple selection

FileOpenMulti

File Save

FileSave

Folder Select

SelectFolder

Folder Select Multiple:

SelectFolderMulti

MessageBox

MessageBox

Changes

  • Implements IStorageXXX interfaces as ConsoloniaStorageProvider
  • Changes consolewindow to register ConsoloniaStorageProvider
  • Move DialogHelper classes to Core library in Controls. This is because there is no way for an external assembly to register for IStorage, and if IStorageProvider is in Core library it can't have dependency on Turbo library
  • Created Controls folder and Controls/Dialog folder
  • Moved DialogWrap/Dialog helper classes into Controls/Dialogs* Moved DialogWindow into Controls, this makes DialogWindow a core control for Consolonia
  • Added FileOpenPicker dialog with view models
  • Added FileSavePicker Dialog with view models
  • Added FolderPicker dialog with view models
  • Added GalleryStorage to illustrate calling IStorageProvider to show FileOpen/FileSave/SelectFolder
  • Cleaned up DialogWindow template to support Icon as a string Emoticon, cleaned up layout of DialogWindow a bit.
  • Added unit tests for StorageProvider/StorageFile/StorageFolder

NOTE: The icon rendering display issues are already is fixed with PR #150

Summary by CodeRabbit

Release Notes

  • New Features

    • Introduced FileOpenPicker, FileSavePicker, and FolderPicker controls for enhanced file and folder selection.
    • Added a customizable MessageBox control for user interactions.
    • Implemented GalleryStorage and GalleryMessageBox for managing file operations in the gallery.
  • Improvements

    • Enhanced DialogWindow and DialogHost functionality for better dialog management.
    • Updated DialogWindow properties for improved configuration options.
  • Bug Fixes

    • Resolved issues with dialog visibility and interaction.
  • Refactor

    • Streamlined initialization processes by removing redundant InitializeComponent methods across various controls.
    • Improved code clarity by directly accessing control properties instead of using FindControl.
  • Documentation

    • Updated project files and configurations for better organization and clarity.

Copy link
Contributor

coderabbitai bot commented Nov 25, 2024

Walkthrough

The changes in this pull request encompass various updates across multiple files within the Consolonia project. Key modifications include the restructuring of namespaces, the introduction of new controls such as FileOpenPicker, FileSavePicker, and MessageBox, and enhancements to existing classes like DialogWindow and ConsoloniaStorageProvider. The project files have been updated to include new package references and suppress specific warnings. Additionally, several InitializeComponent methods have been removed, indicating a shift in how XAML components are initialized.

Changes

File Path Change Summary
src/Consolonia.Core/Consolonia.Core.csproj Added <PackageReference Include="CommunityToolkit.Mvvm" Version="8.3.2" /> and removed various item types from Controls\FileOpen.
src/Consolonia.Core/Controls/Dialog/DialogHelpers.cs Changed namespace from Consolonia.Themes.TurboVision.Templates.Controls.Dialog to Consolonia.Core.Controls.Dialog.
src/Consolonia.Core/Controls/Dialog/DialogWrap.axaml Updated x:Class from Consolonia.Themes.TurboVision.Templates.Controls.DialogWrap to Consolonia.Core.Controls.Dialog.DialogWrap.
src/Consolonia.Core/Controls/Dialog/DialogWrap.axaml.cs Changed namespace from Consolonia.Themes.TurboVision.Templates.Controls to Consolonia.Core.Controls.Dialog.
src/Consolonia.Core/Controls/DialogWindow.cs Changed namespace, added new properties, and modified method signatures for dialog handling.
src/Consolonia.Core/Controls/FileOpenPicker.axaml Introduced new FileOpenPicker control for file selection.
src/Consolonia.Core/Controls/FileOpenPicker.axaml.cs Implemented FileOpenPicker class with constructors and event handlers.
src/Consolonia.Core/Controls/FileOpenPickerViewModel.cs Added FileOpenPickerViewModel class for managing file picker state.
src/Consolonia.Core/Controls/FileSavePicker.axaml Introduced new FileSavePicker control for saving files.
src/Consolonia.Core/Controls/FileSavePicker.axaml.cs Implemented FileSavePicker class with constructors and event handlers.
src/Consolonia.Core/Controls/FileSavePickerViewModel.cs Enhanced FileSavePickerViewModel class with new properties and event handling.
src/Consolonia.Core/Controls/FolderPicker.axaml Introduced new FolderPicker control for folder selection.
src/Consolonia.Core/Controls/FolderPicker.axaml.cs Implemented FolderPicker class with constructors and event handlers.
src/Consolonia.Core/Controls/FolderPickerViewModel.cs Added FolderPickerViewModel class for managing folder picker state.
src/Consolonia.Core/Infrastructure/ConsoloniaStorageProvider.cs Implemented ConsoloniaStorageProvider class for file and folder operations.
src/Consolonia.Gallery/App.cs Added RegisterServices method in App class.
src/Consolonia.Gallery/Gallery/GalleryViews/GalleryDialog.axaml.cs Modified dialog display context to use the main application window.
src/Consolonia.Gallery/Gallery/GalleryViews/GalleryStorage.axaml Introduced new GalleryStorage control for file and folder management.
src/Consolonia.Gallery/Gallery/GalleryViews/GalleryStorage.axaml.cs Implemented GalleryStorageViewModel and event handlers for user interactions.
src/Consolonia.Gallery/Gallery/GalleryViews/GalleryMessageBox.axaml Introduced new MessageBox control for user interactions.
src/Consolonia.Gallery/Gallery/GalleryViews/GalleryMessageBox.axaml.cs Implemented MessageBox class with properties and methods for dialog interactions.
src/Consolonia.Gallery/Gallery/GalleryViews/GalleryAnimatedLines.axaml.cs Removed InitializeComponent method, indicating a change in initialization.
src/Consolonia.Gallery/Gallery/GalleryViews/GalleryButton.axaml.cs Removed InitializeComponent method, indicating a change in initialization.
src/Consolonia.Gallery/Gallery/GalleryViews/GalleryCalendar.axaml.cs Removed InitializeComponent method, indicating a change in initialization.
src/Consolonia.Gallery/Gallery/GalleryViews/GalleryCalendarPicker.axaml.cs Removed InitializeComponent method, indicating a change in initialization.
src/Consolonia.Gallery/Gallery/GalleryViews/GalleryCanvas.axaml.cs Removed InitializeComponent method, indicating a change in initialization.
src/Consolonia.Gallery/Gallery/GalleryViews/GalleryCheckBox.axaml.cs Removed InitializeComponent method, indicating a change in initialization.
src/Consolonia.Gallery/Gallery/GalleryViews/GalleryColors.axaml.cs Removed InitializeComponent method, indicating a change in initialization.
src/Consolonia.Gallery/Gallery/GalleryViews/GalleryComboBox.axaml.cs Removed InitializeComponent method and streamlined control access.
src/Consolonia.Gallery/Gallery/GalleryViews/GalleryDataGrid.axaml.cs Removed InitializeComponent method and simplified control access.
src/Consolonia.Gallery/Gallery/GalleryViews/GalleryFlyout.axaml.cs Removed InitializeComponent method, indicating a change in initialization.
src/Consolonia.Gallery/Gallery/GalleryViews/GalleryGradientBrush.axaml.cs Removed InitializeComponent method and simplified control access.
src/Consolonia.Gallery/Gallery/GalleryViews/GalleryImage.axaml.cs Removed InitializeComponent method, indicating a change in initialization.
src/Consolonia.Gallery/Gallery/GalleryViews/GalleryListBox.axaml.cs Removed InitializeComponent method and simplified control access.
src/Consolonia.Gallery/Gallery/GalleryViews/GalleryMenu.axaml.cs Removed InitializeComponent method, indicating a change in initialization.
src/Consolonia.Gallery/Gallery/GalleryViews/GalleryProgressBar.axaml.cs Removed InitializeComponent method, indicating a change in initialization.
src/Consolonia.Gallery/Gallery/GalleryViews/GalleryRadioButton.axaml.cs Removed InitializeComponent method, indicating a change in initialization.
src/Consolonia.Gallery/Gallery/GalleryViews/GalleryScrollViewer.axaml.cs Removed InitializeComponent method and several unused directives.
src/Consolonia.Gallery/Gallery/GalleryViews/GalleryTabControl.axaml.cs Removed InitializeComponent method, indicating a change in initialization.
src/Consolonia.Gallery/Gallery/GalleryViews/GalleryTextBlock.axaml.cs Removed InitializeComponent method, indicating a change in initialization.
src/Consolonia.Gallery/Gallery/GalleryViews/GalleryTextBox.axaml.cs Removed InitializeComponent method, indicating a change in initialization.
src/Consolonia.Gallery/Gallery/GalleryViews/GalleryWelcome.axaml.cs Removed InitializeComponent method, indicating a change in initialization.
src/Consolonia.Gallery/View/ControlsListView.axaml.cs Removed _grid field and simplified control access.
src/Example/App.axaml.cs Removed Initialize method, indicating a change in resource loading.
src/Example/Views/DataGridTestWindow.axaml Updated event handler from delete_Clicked to Delete_Clicked.
src/Example/Views/DataGridTestWindow.axaml.cs Simplified control access and renamed event handler.
src/Consolonia.GuiCS/Consolonia.GuiCS.csproj Adjusted indentation for consistency in project file.
Directory.Build.props Added <NoWarn>AVA3001</NoWarn> to suppress specific warnings during build.
src/.editorconfig Changed severity for dotnet_diagnostic.CA2007 to none.

Poem

In the garden of code, changes bloom bright,
New dialogs and pickers, a wondrous sight.
With namespaces shifting, and methods anew,
The rabbit hops on, with a joyful view.
Let’s celebrate progress, with a hop and a cheer,
For Consolonia's magic is now crystal clear! 🐇✨


Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 6

🧹 Outside diff range and nitpick comments (7)
src/Consolonia.Core/Controls/FolderPicker.axaml.cs (3)

10-15: Add XML documentation for public members.

As this is a public class, adding XML documentation would improve API discoverability and usage understanding.

+    /// <summary>
+    /// A dialog window that allows users to select one or multiple folders.
+    /// </summary>
     public partial class FolderPicker : DialogWindow
     {
+        /// <summary>
+        /// Initializes a new instance of the FolderPicker with default options.
+        /// </summary>
         public FolderPicker()
             : this(new FolderPickerOpenOptions())
         {
         }

48-56: Add error handling to dialog operations.

The OK and Cancel handlers should include error handling for the dialog operations.

     private void OnOK(object sender, RoutedEventArgs e)
     {
+        try
+        {
             CloseDialog(ViewModel.SelectedFolders);
+        }
+        catch (Exception ex)
+        {
+            // Log error and show user-friendly message
+            // Logger.LogError(ex, "Failed to close dialog with selection");
+        }
     }

     private void OnCancel(object sender, RoutedEventArgs e)
     {
+        try
+        {
             CloseDialog();
+        }
+        catch (Exception ex)
+        {
+            // Log error and show user-friendly message
+            // Logger.LogError(ex, "Failed to cancel dialog");
+        }
     }

58-81: Refactor ListBox_SelectionChanged for better maintainability.

The selection changed handler is complex and could benefit from being split into smaller, more focused methods.

     private void ListBox_SelectionChanged(object sender, SelectionChangedEventArgs e)
     {
+        try
+        {
-            if (ViewModel.SelectionMode == SelectionMode.Single)
-            {
-                if (e.AddedItems.Count > 0 &&
-                    e.AddedItems[0] is IStorageFolder folder)
-                {
-                    ViewModel.SelectedFolders.Clear();
-                    ViewModel.SelectedFolders.Add(folder);
-                }
-            }
-            else
-            {
-                foreach (object item in e.AddedItems)
-                    if (item is IStorageFolder folder)
-                        ViewModel.SelectedFolders.Add(folder);
-
-                foreach (object item in e.RemovedItems)
-                    if (item is IStorageFolder folder)
-                        ViewModel.SelectedFolders.Remove(folder);
-            }
+            UpdateSelection(e);
+            UpdateSelectionState();
+        }
+        catch (Exception ex)
+        {
+            // Log error and show user-friendly message
+            // Logger.LogError(ex, "Failed to update selection");
+        }
+    }

-            ViewModel.HasSelection = ViewModel.SelectedFolders.Count > 0;
+    private void UpdateSelection(SelectionChangedEventArgs e)
+    {
+        if (ViewModel.SelectionMode == SelectionMode.Single)
+        {
+            HandleSingleSelection(e.AddedItems);
+        }
+        else
+        {
+            HandleMultiSelection(e.AddedItems, e.RemovedItems);
+        }
+    }
+
+    private void HandleSingleSelection(IList addedItems)
+    {
+        if (addedItems.Count > 0 && addedItems[0] is IStorageFolder folder)
+        {
+            ViewModel.SelectedFolders.Clear();
+            ViewModel.SelectedFolders.Add(folder);
+        }
+    }
+
+    private void HandleMultiSelection(IList addedItems, IList removedItems)
+    {
+        foreach (var folder in addedItems.OfType<IStorageFolder>())
+        {
+            ViewModel.SelectedFolders.Add(folder);
+        }
+
+        foreach (var folder in removedItems.OfType<IStorageFolder>())
+        {
+            ViewModel.SelectedFolders.Remove(folder);
+        }
+    }
+
+    private void UpdateSelectionState()
+    {
+        ViewModel.HasSelection = ViewModel.SelectedFolders.Count > 0;
     }
src/Consolonia.Core/Controls/MessageBox.axaml.cs (4)

10-123: Add XML documentation comments to public members

To enhance maintainability and improve developer experience, consider adding XML documentation comments to public classes, methods, properties, and enums. This will aid in generating API documentation and provide better IntelliSense support.


88-96: Handle potential null values for 'text' and 'title' parameters

In the ShowDialogAsync method, if text or title is null, it might lead to unexpected behavior or exceptions in the dialog display. Consider validating these parameters or ensuring that the MessageBoxViewModel handles null values appropriately.


91-95: Consider enabling nullable reference types at the project level

Instead of enabling #nullable enable within a specific method, consider enabling nullable reference types at the project or file level. This provides consistent nullability analysis across the codebase and helps prevent null-related issues.


156-211: Consider sealing the 'MessageBoxViewModel' class

If MessageBoxViewModel is not intended for inheritance, marking it as sealed can prevent unintended subclassing and allow for potential performance optimizations.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between f2e6352 and a4a4468.

📒 Files selected for processing (7)
  • src/Consolonia.Core/Controls/FileOpenPicker.axaml.cs (1 hunks)
  • src/Consolonia.Core/Controls/FileSavePicker.axaml.cs (1 hunks)
  • src/Consolonia.Core/Controls/FolderPicker.axaml.cs (1 hunks)
  • src/Consolonia.Core/Controls/MessageBox.axaml (1 hunks)
  • src/Consolonia.Core/Controls/MessageBox.axaml.cs (1 hunks)
  • src/Consolonia.Themes.TurboVision/Templates/Controls/DialogWindow.axaml (3 hunks)
  • src/Consolonia.Themes.TurboVision/Templates/Controls/Helpers/SymbolsControl.cs (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
  • src/Consolonia.Core/Controls/MessageBox.axaml
  • src/Consolonia.Themes.TurboVision/Templates/Controls/DialogWindow.axaml
  • src/Consolonia.Themes.TurboVision/Templates/Controls/Helpers/SymbolsControl.cs
🔇 Additional comments (3)
src/Consolonia.Core/Controls/FileSavePicker.axaml.cs (1)

1-98: LGTM!

The implementation of FileSavePicker is well-structured and follows good coding practices.

src/Consolonia.Core/Controls/MessageBox.axaml.cs (2)

6-6: Verify the introduction of new dependencies in the Core library

Including CommunityToolkit.Mvvm.ComponentModel introduces a dependency on the Community Toolkit. Ensure that adding this dependency aligns with the project's goal of maintaining a lightweight Core library without unnecessary external dependencies.


45-54: Ensure that the buttons being focused exist in the XAML

In the Loaded event handler, the code attempts to find OkButton, CancelButton, YesButton, or NoButton controls to set the focus. Verify that these buttons are correctly named and exist in the associated XAML file. If none are found, the focus operation may silently fail.

tomlm and others added 5 commits November 26, 2024 12:48
…ed in 11.x avalonia but it breaks initalization of named controls.

This is why we had to do .FindControl() instead of just the x:Name of the control.
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🧹 Outside diff range and nitpick comments (20)
src/Consolonia.Gallery/Gallery/GalleryViews/GalleryComboBox.axaml.cs (1)

19-19: Add error handling for empty environment variables list.

Setting SelectedIndex = 0 without checking if the list is empty could cause an exception.

Consider adding a null check:

-            VarsComboBox.SelectedIndex = 0;
+            if (VarsComboBox.ItemsSource?.Cast<object>().Any() == true)
+                VarsComboBox.SelectedIndex = 0;
src/Consolonia.Gallery/Gallery/GalleryViews/GalleryCalendar.axaml.cs (1)

Line range hint 13-18: Add bounds checking for date calculations

The date calculations (AddDays) could potentially overflow if the values are near DateTime bounds. Consider adding validation:

-            cal1.DisplayDateStart = today.AddDays(-25);
-            cal1.DisplayDateEnd = today.AddDays(25);
+            // Ensure date calculations don't overflow
+            if (today > DateTime.MinValue.AddDays(25) && today < DateTime.MaxValue.AddDays(-25))
+            {
+                cal1.DisplayDateStart = today.AddDays(-25);
+                cal1.DisplayDateEnd = today.AddDays(25);
+            }
+            else
+            {
+                // Handle edge case or log warning
+                cal1.DisplayDateStart = today;
+                cal1.DisplayDateEnd = today;
+            }
src/Example/Views/DataGridTestWindow.axaml.cs (1)

46-49: Consider adding null checks and improving error handling.

While the event handler follows proper naming conventions, the casting could be more defensive:

-private void Delete_Clicked(object sender, RoutedEventArgs _)
-{
-    _items.Remove((TheItem)((Control)sender).DataContext);
-}
+private void Delete_Clicked(object sender, RoutedEventArgs _)
+{
+    if (sender is Control control && control.DataContext is TheItem item)
+    {
+        _items.Remove(item);
+    }
+}

Also, consider addressing the TODO comment about the ReSharper rule suppression.

src/Consolonia.Gallery/Gallery/GalleryViews/GalleryCalendarPicker.axaml.cs (4)

Line range hint 16-19: Consider using XAML bindings instead of direct control references

Retrieving controls by string IDs is fragile and error-prone. Consider:

  1. Using proper XAML bindings to view model properties
  2. Using x:Name references if direct access is necessary

Line range hint 21-23: Move date initialization to view model

Setting dates directly in the control's constructor violates MVVM principles. Consider:

  1. Moving these default values to the view model
  2. Using XAML bindings to connect the view model properties to the CalendarDatePicker controls

Line range hint 25-25: Consider moving blackout dates logic to view model

The blackout dates configuration is UI logic that could be better managed in the view model:

  1. It would improve testability
  2. It would make the behavior more maintainable

Line range hint 1-54: Consider restructuring to better follow MVVM pattern

The current implementation mixes view and view model concerns. Consider:

  1. Moving all date manipulation logic to the view model
  2. Using XAML bindings for control properties
  3. Creating a dedicated interface for the view model
  4. Adding commands for user interactions

This would improve:

  • Testability
  • Maintainability
  • Separation of concerns
src/Consolonia.Gallery/View/ControlsListView.axaml.cs (1)

32-32: Consider adding null check for Grid property.

The changes to use this.Grid directly are good, but consider adding a null check for defensive programming.

 private void TrySetupSelected()
 {
+    if (this.Grid == null)
+        throw new InvalidOperationException("Grid control not initialized");
+
     if (_commandLineArgs.Length is not 1 and not 2)
     {
         this.Grid.SelectedIndex = 0;
         return;
     }

Also applies to: 52-53

src/Consolonia.Gallery/Gallery/GalleryViews/GalleryGradientBrush.axaml.cs (4)

Line range hint 9-12: Fix initialization after InitializeComponent removal

The constructor still calls InitializeComponent() but according to the summary, this method has been removed. This will cause a compilation error.

Apply this diff to fix the initialization:

 public GalleryGradientBrush()
 {
-    InitializeComponent();
 }

Line range hint 30-40: Add null check for consistency

For consistency with the previous suggestion, consider adding a null check here as well.

Apply this diff:

 private void Radial_Click(object sender, Avalonia.Interactivity.RoutedEventArgs e)
 {
+    if (MyGrid == null) return;
     this.MyGrid.Background = new RadialGradientBrush

43-44: Remove extra blank line

There's an extra blank line that affects code formatting consistency.

         }
-
         private void Conic_Click(object sender, Avalonia.Interactivity.RoutedEventArgs e)

Line range hint 45-58: Add null check for consistency

For consistency with the previous suggestions, consider adding a null check here as well.

Apply this diff:

 private void Conic_Click(object sender, Avalonia.Interactivity.RoutedEventArgs e)
 {
+    if (MyGrid == null) return;
     this.MyGrid.Background = new ConicGradientBrush
src/Consolonia.Core/Controls/Dialog/DialogWrap.axaml.cs (2)

Line range hint 25-39: Consider potential memory leak in event subscription

The AttachedToVisualTree handler creates a new subscription each time it fires, but the previous subscription is only disposed when the control is detached. This could lead to multiple active subscriptions if the control is repeatedly attached without being detached.

Consider this safer implementation:

  AttachedToVisualTree += (_, _) =>
  {
+     _disposable?.Dispose();
      var parentWindow = this.FindAncestorOfType<Window>();
      _disposable = parentWindow!.GetPropertyChangedObservable(TopLevel.ClientSizeProperty).Subscribe(
          new AnonymousObserver<AvaloniaPropertyChangedEventArgs>(
              args =>
              {
                  var newSize = (Size)args.NewValue!;
                  SetNewSize(newSize);
              }));
      SetNewSize(parentWindow!.ClientSize);
  };

Line range hint 63-67: Reconsider commented code retention

The CloseDialog method is marked as unused and suppressed with pragmas, kept only as an example for future mouse support. Consider either:

  1. Removing it and tracking the requirement in documentation/issues
  2. Implementing it now if mouse support is planned soon

If keeping it for documentation purposes, consider improving the comment:

- // ReSharper disable once UnusedMember.Local Example of usage for further (when mouse support introduced for example)
+ /// <summary>
+ /// Reserved for future mouse support implementation.
+ /// This method will handle dialog closure triggered by mouse interactions.
+ /// </summary>
src/Consolonia.Core/Controls/FolderPicker.axaml.cs (1)

1-77: Consider implementing MVVM pattern more strictly.

The current implementation has tight coupling between the view and view model through direct property access. Consider:

  1. Using commands instead of event handlers
  2. Implementing proper data binding
  3. Moving selection logic to the view model

This would improve testability and maintainability. For example:

+ // In ViewModel
+ public ICommand SelectFolderCommand { get; }
+ public ICommand CancelCommand { get; }
+ public ICommand UpdateSelectionCommand { get; }

+ // In View
- private void OnOK(object sender, RoutedEventArgs e)
- {
-     CloseDialog(ViewModel.SelectedFolders);
- }
+ <Button Command="{Binding SelectFolderCommand}" />

Would you like assistance in implementing these architectural improvements?

src/Example/Views/DataGridTestWindow.axaml (1)

Line range hint 8-106: Consider enhancing accessibility with ARIA labels.

While the UI has good keyboard navigation support, consider adding aria-label attributes to improve accessibility for screen readers, particularly for the data grid and action buttons.

Add aria labels to key interactive elements:

             <DataGrid ItemsSource="{Binding}"
                      Name="Grid"
                      SelectionMode="Single"
                      Focusable="True"
                      AutoGenerateColumns="False"
+                     AutomationProperties.Name="Movie Catalog Grid"
                      HorizontalScrollBarVisibility="Disabled">

             <!-- ... -->

             <Button Content="Delete"
                     Grid.Row="4"
                     Grid.ColumnSpan="2"
                     Grid.Column="0"
                     HorizontalAlignment="Center"
+                    AutomationProperties.Name="Delete Selected Movie"
                     Click="Delete_Clicked" />
src/Consolonia.Gallery/Gallery/GalleryViews/GalleryDataGrid.axaml.cs (2)

Line range hint 32-43: Consider simplifying the sorting logic

The sorting logic in the event handler could be simplified and made more maintainable.

Consider this refactoring:

-DataGrid1.Sorting += (_, a) =>
-{
-    var binding = (a.Column as DataGridBoundColumn)?.Binding as Binding;
-
-    if (binding?.Path is { } property
-        && property == dataGridSortDescription.PropertyPath
-        && !collectionView1.SortDescriptions.Contains(dataGridSortDescription))
-        collectionView1.SortDescriptions.Add(dataGridSortDescription);
-};
+DataGrid1.Sorting += (_, args) =>
+{
+    if (args.Column is DataGridBoundColumn { Binding: Binding binding } 
+        && binding.Path == dataGridSortDescription.PropertyPath
+        && !collectionView1.SortDescriptions.Contains(dataGridSortDescription))
+    {
+        collectionView1.SortDescriptions.Add(dataGridSortDescription);
+    }
+};

Line range hint 52-62: Consider separating sample data and adding error handling

The current implementation could be improved in terms of separation of concerns and error handling.

  1. Move the sample data to a separate method or service:
+private static List<Person> GetSamplePersons()
+{
+    return new List<Person>
+    {
+        new() { FirstName = "John", LastName = "Doe" },
+        new() { FirstName = "Elizabeth", LastName = "Thomas", IsBanned = true },
+        new() { FirstName = "Zack", LastName = "Ward" }
+    };
+}
  1. Add error handling to the Add button click handler:
-BtnAdd.Click += (_, _) => collectionView3.AddNew();
+BtnAdd.Click += (_, _) =>
+{
+    try
+    {
+        collectionView3.AddNew();
+    }
+    catch (Exception ex)
+    {
+        // Handle or log the error appropriately
+        Debug.WriteLine($"Failed to add new item: {ex.Message}");
+    }
+};
src/Consolonia.Core/Controls/MessageBox.axaml.cs (2)

56-66: Add XML documentation comments to public properties and methods

To improve code maintainability and assist developers using this class, consider adding XML documentation comments to all public properties and methods.

For example:

+/// <summary>
+/// Gets or sets the message box mode, which determines the buttons displayed.
+/// </summary>
 public Mode Mode
 {
     get => _mode;
     set => SetAndRaise(ModeProperty, ref _mode, value);
 }

Also applies to: 68-84, 86-94


9-9: Remove partial modifier if not required

The classes MessageBox and MessageBoxViewModel are declared with the partial keyword, but there are no other partial class definitions provided. If these classes are not split across multiple files, consider removing the partial modifier for clarity.

Apply this change:

-public partial class MessageBox : DialogWindow
+public class MessageBox : DialogWindow
-internal partial class MessageBoxViewModel : ObservableObject
+internal class MessageBoxViewModel : ObservableObject

Also applies to: 148-148

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 519d451 and fe97462.

📒 Files selected for processing (37)
  • src/Consolonia.Core/Controls/Dialog/DialogWrap.axaml.cs (1 hunks)
  • src/Consolonia.Core/Controls/FileOpenPicker.axaml.cs (1 hunks)
  • src/Consolonia.Core/Controls/FileSavePicker.axaml.cs (1 hunks)
  • src/Consolonia.Core/Controls/FileSavePickerViewModel.cs (1 hunks)
  • src/Consolonia.Core/Controls/FolderPicker.axaml (1 hunks)
  • src/Consolonia.Core/Controls/FolderPicker.axaml.cs (1 hunks)
  • src/Consolonia.Core/Controls/MessageBox.axaml.cs (1 hunks)
  • src/Consolonia.Gallery/App.cs (1 hunks)
  • src/Consolonia.Gallery/Gallery/GalleryViews/GalleryAnimatedLines.axaml.cs (0 hunks)
  • src/Consolonia.Gallery/Gallery/GalleryViews/GalleryButton.axaml.cs (0 hunks)
  • src/Consolonia.Gallery/Gallery/GalleryViews/GalleryCalendar.axaml.cs (1 hunks)
  • src/Consolonia.Gallery/Gallery/GalleryViews/GalleryCalendarPicker.axaml.cs (1 hunks)
  • src/Consolonia.Gallery/Gallery/GalleryViews/GalleryCanvas.axaml.cs (0 hunks)
  • src/Consolonia.Gallery/Gallery/GalleryViews/GalleryCheckBox.axaml.cs (0 hunks)
  • src/Consolonia.Gallery/Gallery/GalleryViews/GalleryColors.axaml.cs (0 hunks)
  • src/Consolonia.Gallery/Gallery/GalleryViews/GalleryComboBox.axaml.cs (1 hunks)
  • src/Consolonia.Gallery/Gallery/GalleryViews/GalleryDataGrid.axaml.cs (3 hunks)
  • src/Consolonia.Gallery/Gallery/GalleryViews/GalleryDialog.axaml.cs (2 hunks)
  • src/Consolonia.Gallery/Gallery/GalleryViews/GalleryFlyout.axaml.cs (0 hunks)
  • src/Consolonia.Gallery/Gallery/GalleryViews/GalleryGradientBrush.axaml.cs (3 hunks)
  • src/Consolonia.Gallery/Gallery/GalleryViews/GalleryImage.axaml.cs (0 hunks)
  • src/Consolonia.Gallery/Gallery/GalleryViews/GalleryListBox.axaml.cs (0 hunks)
  • src/Consolonia.Gallery/Gallery/GalleryViews/GalleryMenu.axaml.cs (0 hunks)
  • src/Consolonia.Gallery/Gallery/GalleryViews/GalleryMessgeBox.axaml.cs (1 hunks)
  • src/Consolonia.Gallery/Gallery/GalleryViews/GalleryProgressBar.axaml.cs (0 hunks)
  • src/Consolonia.Gallery/Gallery/GalleryViews/GalleryRadioButton.axaml.cs (0 hunks)
  • src/Consolonia.Gallery/Gallery/GalleryViews/GalleryScrollViewer.axaml.cs (0 hunks)
  • src/Consolonia.Gallery/Gallery/GalleryViews/GalleryStorage.axaml.cs (1 hunks)
  • src/Consolonia.Gallery/Gallery/GalleryViews/GalleryTabControl.axaml.cs (0 hunks)
  • src/Consolonia.Gallery/Gallery/GalleryViews/GalleryTextBlock.axaml.cs (0 hunks)
  • src/Consolonia.Gallery/Gallery/GalleryViews/GalleryTextBox.axaml.cs (0 hunks)
  • src/Consolonia.Gallery/Gallery/GalleryViews/GalleryWelcome.axaml.cs (0 hunks)
  • src/Consolonia.Gallery/Gallery/GalleryViews/SomeDialogWindow.axaml.cs (2 hunks)
  • src/Consolonia.Gallery/View/ControlsListView.axaml.cs (3 hunks)
  • src/Example/App.axaml.cs (0 hunks)
  • src/Example/Views/DataGridTestWindow.axaml (1 hunks)
  • src/Example/Views/DataGridTestWindow.axaml.cs (2 hunks)
💤 Files with no reviewable changes (17)
  • src/Consolonia.Gallery/Gallery/GalleryViews/GalleryAnimatedLines.axaml.cs
  • src/Consolonia.Gallery/Gallery/GalleryViews/GalleryButton.axaml.cs
  • src/Consolonia.Gallery/Gallery/GalleryViews/GalleryCanvas.axaml.cs
  • src/Consolonia.Gallery/Gallery/GalleryViews/GalleryCheckBox.axaml.cs
  • src/Consolonia.Gallery/Gallery/GalleryViews/GalleryColors.axaml.cs
  • src/Consolonia.Gallery/Gallery/GalleryViews/GalleryFlyout.axaml.cs
  • src/Consolonia.Gallery/Gallery/GalleryViews/GalleryImage.axaml.cs
  • src/Consolonia.Gallery/Gallery/GalleryViews/GalleryListBox.axaml.cs
  • src/Consolonia.Gallery/Gallery/GalleryViews/GalleryMenu.axaml.cs
  • src/Consolonia.Gallery/Gallery/GalleryViews/GalleryProgressBar.axaml.cs
  • src/Consolonia.Gallery/Gallery/GalleryViews/GalleryRadioButton.axaml.cs
  • src/Consolonia.Gallery/Gallery/GalleryViews/GalleryScrollViewer.axaml.cs
  • src/Consolonia.Gallery/Gallery/GalleryViews/GalleryTabControl.axaml.cs
  • src/Consolonia.Gallery/Gallery/GalleryViews/GalleryTextBlock.axaml.cs
  • src/Consolonia.Gallery/Gallery/GalleryViews/GalleryTextBox.axaml.cs
  • src/Consolonia.Gallery/Gallery/GalleryViews/GalleryWelcome.axaml.cs
  • src/Example/App.axaml.cs
🚧 Files skipped from review as they are similar to previous changes (9)
  • src/Consolonia.Core/Controls/FileOpenPicker.axaml.cs
  • src/Consolonia.Core/Controls/FileSavePicker.axaml.cs
  • src/Consolonia.Core/Controls/FileSavePickerViewModel.cs
  • src/Consolonia.Core/Controls/FolderPicker.axaml
  • src/Consolonia.Gallery/App.cs
  • src/Consolonia.Gallery/Gallery/GalleryViews/GalleryDialog.axaml.cs
  • src/Consolonia.Gallery/Gallery/GalleryViews/GalleryMessgeBox.axaml.cs
  • src/Consolonia.Gallery/Gallery/GalleryViews/GalleryStorage.axaml.cs
  • src/Consolonia.Gallery/Gallery/GalleryViews/SomeDialogWindow.axaml.cs
🔇 Additional comments (11)
src/Consolonia.Gallery/Gallery/GalleryViews/GalleryCalendar.axaml.cs (1)

Line range hint 11-18: Verify calendar control identifiers in XAML

The code uses string literals "DisplayDatesCalendar" and "BlackoutDatesCalendar" to look up controls. Consider extracting these as constants to prevent typos and enable easier refactoring.

Let's verify these control IDs exist in the XAML:

✅ Verification successful

Control identifiers are correctly defined in XAML

The control identifiers "DisplayDatesCalendar" and "BlackoutDatesCalendar" are properly defined in the XAML file as Name attributes on the respective Calendar controls. The lookup in the code will work as expected.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify calendar control IDs exist in the corresponding XAML file
# Expected: Should find both control IDs in the XAML

rg -l "DisplayDatesCalendar|BlackoutDatesCalendar" --type xaml "src/Consolonia.Gallery/Gallery/GalleryViews/GalleryCalendar.axaml"

Length of output: 159


Script:

#!/bin/bash
# Let's try searching without file type restriction since .xaml might not be recognized
rg -l "DisplayDatesCalendar|BlackoutDatesCalendar" "src/Consolonia.Gallery/Gallery/GalleryViews/GalleryCalendar.axaml"

# Also let's verify if the file exists and see its content
fd "GalleryCalendar.axaml" && cat src/Consolonia.Gallery/Gallery/GalleryViews/GalleryCalendar.axaml

Length of output: 2462

src/Example/Views/DataGridTestWindow.axaml.cs (1)

22-22: LGTM! Direct property access is cleaner.

The direct access to Combo.ItemsSource is more maintainable than using FindControl.

src/Consolonia.Gallery/Gallery/GalleryViews/GalleryCalendarPicker.axaml.cs (2)

Line range hint 32-54: LGTM: Well-implemented view model

The GalleryCalendarPickerDataContext class follows good practices:

  • Proper implementation of INotifyPropertyChanged
  • Clean property change notification
  • Good use of attributes for validation
  • Clear documentation

Line range hint 14-14: Critical: InitializeComponent() is called but was removed

The constructor calls InitializeComponent() which according to the AI summary was removed. This will cause a compilation error.

Let's verify if this is a common pattern across other controls:

src/Consolonia.Gallery/Gallery/GalleryViews/GalleryGradientBrush.axaml.cs (2)

Line range hint 16-25: Verify MyGrid property generation and add null check

The change from FindControl to direct property access assumes that MyGrid is properly generated as a property. Additionally, consider adding a null check before accessing the property.

Run the following script to verify the property generation in the XAML file:

Consider adding a null check:

 private void Linear_Click(object sender, Avalonia.Interactivity.RoutedEventArgs e)
 {
+    if (MyGrid == null) return;
     this.MyGrid.Background = new LinearGradientBrush
✅ Verification successful

Property generation verified, but null check recommended

The MyGrid property is correctly set up through Avalonia's code generation:

  • The XAML file defines <Grid x:Name="MyGrid">
  • The class is marked as partial and inherits from UserControl
  • The property will be auto-generated by Avalonia's build process

However, the null check suggestion is still valid as a defensive programming practice:

 private void Linear_Click(object sender, Avalonia.Interactivity.RoutedEventArgs e)
 {
+    if (MyGrid == null) return;
     this.MyGrid.Background = new LinearGradientBrush
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify MyGrid property generation in XAML
# Look for x:Name="MyGrid" in the corresponding XAML file

fd -e axaml "GalleryGradientBrush.axaml$" -x cat {} | grep -A 5 'x:Name="MyGrid"'

Length of output: 383


Script:

#!/bin/bash
# Let's verify if the code is using Avalonia's code generation by checking for the presence of g.cs files
fd -e g.cs . -x cat {} | grep -A 5 "MyGrid"

# Also check if there are any other usages of MyGrid in the codebase
rg "MyGrid" -A 3 -B 3

Length of output: 3401


Script:

#!/bin/bash
# Let's check if this is an Avalonia project and verify the code generation setup
fd "GalleryGradientBrush" -e csproj -x cat {}

# Also check the base class of GalleryGradientBrush to understand the code generation pattern
rg "class GalleryGradientBrush" -A 2

Length of output: 412


Line range hint 1-61: Verify consistent refactoring pattern across similar files

This file shows a pattern of replacing FindControl with direct property access. Let's verify this pattern is consistently applied across other similar files.

✅ Verification successful

Let me gather more information about the refactoring pattern by checking the XAML files and their corresponding code-behind files.


Let me try one more search with a different approach to check the XAML files and FindControl patterns.


Based on the search results, I can now generate the final response:

No FindControl usage found, direct property access pattern is consistent

The verification shows:

  • No instances of FindControl<T>() were found in the codebase
  • The direct property access pattern (e.g., this.MyGrid) is consistently used in the GalleryGradientBrush and other files
  • All UI controls properly use InitializeComponent() in their constructors
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for remaining FindControl usage in other files
# Look for any remaining FindControl patterns that might have been missed

# Search for FindControl usage
echo "Files still using FindControl:"
rg "FindControl<\w+>\(" --type cs

# Search for InitializeComponent calls
echo -e "\nFiles still calling InitializeComponent:"
rg "InitializeComponent\(\)" --type cs

Length of output: 3461


Script:

#!/bin/bash
# Check for MyGrid usage pattern in XAML and code files
echo "Checking MyGrid usage in XAML files:"
rg "Name=\"MyGrid\"" --type xaml

echo -e "\nChecking direct property access patterns similar to 'this.MyGrid' in cs files:"
rg "this\.\w+\.(Background|Content|Children)" --type cs -A 2

echo -e "\nChecking XAML files with x:Name properties:"
rg "x:Name=\"\w+\"" --type xaml

Length of output: 1677


Script:

#!/bin/bash
# Search for .axaml files and their content
echo "Checking .axaml files content:"
fd -e axaml -x cat {}

echo -e "\nChecking for any remaining FindControl patterns in the codebase:"
rg -i "findcontrol" --type-add 'cs:*.{cs,axaml}' --type cs -A 2

Length of output: 101259

src/Consolonia.Core/Controls/Dialog/DialogWrap.axaml.cs (2)

Line range hint 19-23: Verify XAML code generation setup

The class uses InitializeComponent() but the method implementation isn't visible. Ensure the XAML code generation is properly configured for this file.

✅ Verification successful

XAML code generation is properly configured

The verification shows that:

  • The XAML file exists at the correct location (src/Consolonia.Core/Controls/Dialog/DialogWrap.axaml)
  • The XAML file is properly structured with x:Class attribute pointing to the correct class
  • The project file includes Avalonia XAML support
  • The ContentPresenter control is defined in XAML with the correct name that matches the code
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check for corresponding XAML file
fd "DialogWrap.axaml$"

# Check if other dialog controls follow the same pattern
ast-grep --pattern 'class $_ : UserControl { 
  $$$
  InitializeComponent();
  $$$
}'

Length of output: 165


Script:

#!/bin/bash
# Check the content of the XAML file
cat src/Consolonia.Core/Controls/Dialog/DialogWrap.axaml

# Check if there are any build configurations related to XAML
fd ".*proj$" | xargs grep -l "AvaloniaXaml"

# Check for any base class or interface implementations
ast-grep --pattern 'class DialogWrap : $_ {
  $$$
}'

Length of output: 1111


10-10: Verify namespace change impact across the codebase

The namespace change from Consolonia.Themes.TurboVision.Templates.Controls to Consolonia.Core.Controls.Dialog aligns with the PR objective of moving dialog components to Core. However, we should verify the impact on existing consumers.

✅ Verification successful

Namespace change is safe and properly implemented

The namespace change from Consolonia.Themes.TurboVision.Templates.Controls to Consolonia.Core.Controls.Dialog has been properly implemented with corresponding file moves and all references updated. The evidence shows:

  • The file has been physically moved from TurboVision to Core
  • Only one reference to DialogWrap exists in DialogWindow.cs and it's using the new namespace
  • No remaining references to the old namespace were found
  • The XAML file's namespace has been updated accordingly
  • The removal of TurboVision dependency aligns with the goal of maintaining a lightweight Core
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for any remaining references to the old namespace
rg "Consolonia\.Themes\.TurboVision\.Templates\.Controls\.DialogWrap" 

# Search for existing usages that might need updating
rg "using.*DialogWrap" 

Length of output: 96


Script:

#!/bin/bash
# Let's try a broader search for DialogWrap references
rg "DialogWrap" -l

# Search for any imports or using statements containing Dialog
rg "using.*Dialog" 

# Look for XAML files that might reference DialogWrap
fd -e xaml -x grep -l "DialogWrap" {}

# Check git diff to see the changes in namespace usage
git diff origin/main -- "**/DialogWrap*"

Length of output: 2672

src/Example/Views/DataGridTestWindow.axaml (1)

97-97: Verify code-behind updates for the renamed event handler.

The event handler rename from delete_Clicked to Delete_Clicked follows proper C# naming conventions (PascalCase).

Let's verify that the corresponding method in the code-behind file has been updated:

✅ Verification successful

Event handler rename is properly synchronized

The event handler Delete_Clicked exists in both the XAML file and its code-behind file with matching PascalCase naming, confirming proper synchronization between the view and its implementation.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check if the code-behind file has the updated method name

# Find and check the content of the code-behind file
rg -l "DataGridTestWindow\.axaml\.cs$" | xargs rg "Delete_Clicked|delete_Clicked"

Length of output: 296

src/Consolonia.Gallery/Gallery/GalleryViews/GalleryDataGrid.axaml.cs (2)

45-50: LGTM!

The DataGridGrouping setup is clean and follows best practices.


68-71: LGTM!

The event handler rename improves code readability by using a more descriptive name.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (2)
src/Consolonia.Core/Controls/DialogWindow.cs (2)

75-111: Remove or implement commented Position property

The Position property is commented out but includes implementation details. Either implement it if needed or remove it completely to maintain clean code.

-        /// <summary>
-        ///     Gets or sets the window position in screen coordinates.
-        /// </summary>
-        //public PixelPoint Position
-        //{
-        //    get => PlatformImpl?.Position ?? PixelPoint.Origin;
-        //    set => PlatformImpl?.Move(value);
-        //}

Line range hint 187-192: Add null check in GetDialogHost method

The use of null-forgiving operator (!.) might hide potential null reference issues.

 protected static DialogHost GetDialogHost(Visual parent)
 {
     var window = parent.FindAncestorOfType<Window>(true);
+    if (window == null)
+        throw new InvalidOperationException("Dialog must be hosted within a Window.");
-    DialogHost dialogHost = window!.GetValue(DialogHost.DialogHostProperty);
+    DialogHost dialogHost = window.GetValue(DialogHost.DialogHostProperty);
+    if (dialogHost == null)
+        throw new InvalidOperationException("Window must have a DialogHost registered.");
     return dialogHost;
 }
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 331f8e4 and 21972f8.

📒 Files selected for processing (6)
  • src/Consolonia.Core/Controls/DialogWindow.cs (4 hunks)
  • src/Consolonia.Gallery/Gallery/GalleryViews/GalleryDialog.axaml.cs (2 hunks)
  • src/Consolonia.GuiCS/Consolonia.GuiCS.csproj (1 hunks)
  • src/Consolonia.NUnit/Consolonia.NUnit.csproj (1 hunks)
  • src/Consolonia.PlatformSupport/Consolonia.PlatformSupport.csproj (1 hunks)
  • src/Consolonia.Themes.TurboVision/Consolonia.Themes.TurboVision.csproj (1 hunks)
✅ Files skipped from review due to trivial changes (3)
  • src/Consolonia.GuiCS/Consolonia.GuiCS.csproj
  • src/Consolonia.NUnit/Consolonia.NUnit.csproj
  • src/Consolonia.PlatformSupport/Consolonia.PlatformSupport.csproj
🚧 Files skipped from review as they are similar to previous changes (2)
  • src/Consolonia.Gallery/Gallery/GalleryViews/GalleryDialog.axaml.cs
  • src/Consolonia.Themes.TurboVision/Consolonia.Themes.TurboVision.csproj
🧰 Additional context used
📓 Learnings (1)
src/Consolonia.Core/Controls/DialogWindow.cs (1)
Learnt from: tomlm
PR: jinek/Consolonia#153
File: src/Consolonia.Core/Controls/DialogWindow.cs:136-141
Timestamp: 2024-11-25T06:18:08.175Z
Learning: In the `DialogWindow` class (`src/Consolonia.Core/Controls/DialogWindow.cs`), casting `Width` and `Height` to `ushort` is intentional and should not be flagged as an issue.
🔇 Additional comments (5)
src/Consolonia.Core/Controls/DialogWindow.cs (5)

11-11: LGTM! Well-structured property declarations

The namespace change and new property declarations are well-implemented:

  • Proper namespace organization aligning with Core library
  • Correct owner type (DialogWindow) used for property registration
  • Good property naming and default value handling

Also applies to: 15-15, 28-36


134-155: LGTM! Proper window sizing implementation

The window sizing logic is well-implemented with proper event handling and cleanup. The ushort casting is intentional as per previous feedback.


158-164: ⚠️ Potential issue

Add null check for _taskCompletionSource

The CloseDialog method should handle the case where _taskCompletionSource is null to prevent NullReferenceException.

 public virtual void CloseDialog(object result = null)
 {
     DialogHost dialogHost = GetDialogHost(this);
     dialogHost.PopInternal(this);
     if (Parent is Window window) window.SizeChanged -= Window_SizeChanged;
-    _taskCompletionSource.SetResult(result);
+    _taskCompletionSource?.SetResult(result);
 }

Likely invalid or redundant comment.


40-41: 🛠️ Refactor suggestion

Consider using a more type-safe approach for TaskCompletionSource

The current implementation using TaskCompletionSource<object> might lead to runtime casting issues in the generic ShowDialogAsync<T> method.

Consider implementing a type-safe solution:

-private TaskCompletionSource<object> _taskCompletionSource;
+private TaskCompletionSource _taskCompletionSource;
+private Type _expectedResultType;

+protected void SetDialogResult<T>(T result)
+{
+    if (_taskCompletionSource == null)
+        throw new InvalidOperationException("Dialog is not shown.");
+        
+    if (result != null && !_expectedResultType.IsAssignableFrom(result.GetType()))
+        throw new InvalidCastException($"Result type {result.GetType()} is not assignable to expected type {_expectedResultType}");
+        
+    ((TaskCompletionSource<T>)_taskCompletionSource).SetResult(result);
+}

Likely invalid or redundant comment.


176-184: ⚠️ Potential issue

Improve type safety in generic ShowDialogAsync

The unchecked cast to type T could lead to runtime exceptions if the result type doesn't match.

Consider using the type-safe approach with the previously suggested SetDialogResult method:

 public async Task<T> ShowDialogAsync<T>(Control parent)
 {
     if (_taskCompletionSource != null)
         throw new InvalidOperationException("Dialog is already shown.");

-    _taskCompletionSource = new TaskCompletionSource<object>();
+    _taskCompletionSource = new TaskCompletionSource<T>();
+    _expectedResultType = typeof(T);
     ShowDialogInternal(parent);
-    object result = await _taskCompletionSource.Task;
-    return (T)result;
+    return await ((TaskCompletionSource<T>)_taskCompletionSource).Task;
 }

Likely invalid or redundant comment.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (4)
src/Consolonia.Core/Controls/MessageBox.axaml.cs (4)

1-1: Remove unused using directives.

The using directives using System; and using Avalonia.Input; are not required and can be safely removed to clean up the code.

Apply this diff to remove the unused directives:

-using System;
...
-using Avalonia.Input;

Also applies to: 5-5

🧰 Tools
🪛 GitHub Check: build

[warning] 1-1:
"[RedundantUsingDirective] Using directive is not required by the code and can be safely removed" on /home/runner/work/Consolonia/Consolonia/src/Consolonia.Core/Controls/MessageBox.axaml.cs(1,1)


48-48: Remove redundant 'this.' qualifier.

The this. qualifier is redundant here and can be omitted for cleaner code.

Apply this diff:

-             this.AttachedToVisualTree += (s, e) =>
+             AttachedToVisualTree += (s, e) =>
🧰 Tools
🪛 GitHub Check: build

[notice] 48-48:
"[ArrangeThisQualifier] Qualifier 'this.' is redundant" on /home/runner/work/Consolonia/Consolonia/src/Consolonia.Core/Controls/MessageBox.axaml.cs(48,13)


[warning] 48-48:
"[UnusedParameter.Local] Parameter 'e' is never used" on /home/runner/work/Consolonia/Consolonia/src/Consolonia.Core/Controls/MessageBox.axaml.cs(48,46)


[warning] 48-48:
"[UnusedParameter.Local] Parameter 's' is never used" on /home/runner/work/Consolonia/Consolonia/src/Consolonia.Core/Controls/MessageBox.axaml.cs(48,43)


48-48: Rename unused parameters to underscores to indicate they are unused.

The parameters s, e, s2, and e2 are unused in the lambda expressions. Renaming them to _ clarifies that they are intentionally unused.

Apply this diff:

-             AttachedToVisualTree += (s, e) =>
+             AttachedToVisualTree += (_, _) =>

              {
-                 if (OkButton.IsVisible)
-                     OkButton.AttachedToVisualTree += (s2, e2) => OkButton.Focus();
+                 if (OkButton.IsVisible)
+                     OkButton.AttachedToVisualTree += (_, _) => OkButton.Focus();
                  else if (YesButton.IsVisible)
-                     YesButton.AttachedToVisualTree += (s2, e2) => YesButton.Focus();
+                     YesButton.AttachedToVisualTree += (_, _) => YesButton.Focus();
                  else if (CancelButton.IsVisible)
-                     CancelButton.AttachedToVisualTree += (s2, e2) => CancelButton.Focus();
+                     CancelButton.AttachedToVisualTree += (_, _) => CancelButton.Focus();
                  else if (NoButton.IsVisible)
-                     NoButton.AttachedToVisualTree += (s2, e2) => NoButton.Focus();
+                     NoButton.AttachedToVisualTree += (_, _) => NoButton.Focus();
              };

Also applies to: 51-51, 53-53, 55-55, 57-57

🧰 Tools
🪛 GitHub Check: build

[notice] 48-48:
"[ArrangeThisQualifier] Qualifier 'this.' is redundant" on /home/runner/work/Consolonia/Consolonia/src/Consolonia.Core/Controls/MessageBox.axaml.cs(48,13)


[warning] 48-48:
"[UnusedParameter.Local] Parameter 'e' is never used" on /home/runner/work/Consolonia/Consolonia/src/Consolonia.Core/Controls/MessageBox.axaml.cs(48,46)


[warning] 48-48:
"[UnusedParameter.Local] Parameter 's' is never used" on /home/runner/work/Consolonia/Consolonia/src/Consolonia.Core/Controls/MessageBox.axaml.cs(48,43)


95-97: Simplify null-check and return statement with null-coalescing operator.

The null-check can be simplified using the null-coalescing operator ?? for cleaner code.

Apply this diff:

  var result = await ShowDialogAsync<MessageBoxResult?>(parent);
- if (result.HasValue) return result.Value;
- return MessageBoxResult.Cancel;
+ return result ?? MessageBoxResult.Cancel;
🧰 Tools
🪛 GitHub Check: build

[notice] 96-96:
"[ConvertIfStatementToReturnStatement] Convert into 'return' statement" on /home/runner/work/Consolonia/Consolonia/src/Consolonia.Core/Controls/MessageBox.axaml.cs(96,13)

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 21972f8 and 99393d3.

📒 Files selected for processing (1)
  • src/Consolonia.Core/Controls/MessageBox.axaml.cs (1 hunks)
🧰 Additional context used
🪛 GitHub Check: build
src/Consolonia.Core/Controls/MessageBox.axaml.cs

[notice] 48-48:
"[ArrangeThisQualifier] Qualifier 'this.' is redundant" on /home/runner/work/Consolonia/Consolonia/src/Consolonia.Core/Controls/MessageBox.axaml.cs(48,13)


[notice] 96-96:
"[ConvertIfStatementToReturnStatement] Convert into 'return' statement" on /home/runner/work/Consolonia/Consolonia/src/Consolonia.Core/Controls/MessageBox.axaml.cs(96,13)


[warning] 1-1:
"[RedundantUsingDirective] Using directive is not required by the code and can be safely removed" on /home/runner/work/Consolonia/Consolonia/src/Consolonia.Core/Controls/MessageBox.axaml.cs(1,1)


[warning] 5-5:
"[RedundantUsingDirective] Using directive is not required by the code and can be safely removed" on /home/runner/work/Consolonia/Consolonia/src/Consolonia.Core/Controls/MessageBox.axaml.cs(5,1)


[warning] 48-48:
"[UnusedParameter.Local] Parameter 'e' is never used" on /home/runner/work/Consolonia/Consolonia/src/Consolonia.Core/Controls/MessageBox.axaml.cs(48,46)


[warning] 51-51:
"[UnusedParameter.Local] Parameter 'e2' is never used" on /home/runner/work/Consolonia/Consolonia/src/Consolonia.Core/Controls/MessageBox.axaml.cs(51,59)


[warning] 53-53:
"[UnusedParameter.Local] Parameter 'e2' is never used" on /home/runner/work/Consolonia/Consolonia/src/Consolonia.Core/Controls/MessageBox.axaml.cs(53,60)


[warning] 55-55:
"[UnusedParameter.Local] Parameter 'e2' is never used" on /home/runner/work/Consolonia/Consolonia/src/Consolonia.Core/Controls/MessageBox.axaml.cs(55,63)


[warning] 57-57:
"[UnusedParameter.Local] Parameter 'e2' is never used" on /home/runner/work/Consolonia/Consolonia/src/Consolonia.Core/Controls/MessageBox.axaml.cs(57,59)


[warning] 48-48:
"[UnusedParameter.Local] Parameter 's' is never used" on /home/runner/work/Consolonia/Consolonia/src/Consolonia.Core/Controls/MessageBox.axaml.cs(48,43)


[warning] 51-51:
"[UnusedParameter.Local] Parameter 's2' is never used" on /home/runner/work/Consolonia/Consolonia/src/Consolonia.Core/Controls/MessageBox.axaml.cs(51,55)


[warning] 53-53:
"[UnusedParameter.Local] Parameter 's2' is never used" on /home/runner/work/Consolonia/Consolonia/src/Consolonia.Core/Controls/MessageBox.axaml.cs(53,56)

🔇 Additional comments (2)
src/Consolonia.Core/Controls/MessageBox.axaml.cs (2)

171-174: Ensure _text and _title handle null values safely

The fields _text and _title are declared as string but may receive null values. To prevent potential NullReferenceException, declare them as string? or add null checks where they are used.


42-59: Move focus setting logic to the Loaded event handler

Setting focus within the AttachedToVisualTree event may not reliably ensure that controls are fully initialized. Consider moving the focus logic to the Loaded event handler to guarantee all UI elements are ready before setting focus.

🧰 Tools
🪛 GitHub Check: build

[notice] 48-48:
"[ArrangeThisQualifier] Qualifier 'this.' is redundant" on /home/runner/work/Consolonia/Consolonia/src/Consolonia.Core/Controls/MessageBox.axaml.cs(48,13)


[warning] 48-48:
"[UnusedParameter.Local] Parameter 'e' is never used" on /home/runner/work/Consolonia/Consolonia/src/Consolonia.Core/Controls/MessageBox.axaml.cs(48,46)


[warning] 51-51:
"[UnusedParameter.Local] Parameter 'e2' is never used" on /home/runner/work/Consolonia/Consolonia/src/Consolonia.Core/Controls/MessageBox.axaml.cs(51,59)


[warning] 53-53:
"[UnusedParameter.Local] Parameter 'e2' is never used" on /home/runner/work/Consolonia/Consolonia/src/Consolonia.Core/Controls/MessageBox.axaml.cs(53,60)


[warning] 55-55:
"[UnusedParameter.Local] Parameter 'e2' is never used" on /home/runner/work/Consolonia/Consolonia/src/Consolonia.Core/Controls/MessageBox.axaml.cs(55,63)


[warning] 57-57:
"[UnusedParameter.Local] Parameter 'e2' is never used" on /home/runner/work/Consolonia/Consolonia/src/Consolonia.Core/Controls/MessageBox.axaml.cs(57,59)


[warning] 48-48:
"[UnusedParameter.Local] Parameter 's' is never used" on /home/runner/work/Consolonia/Consolonia/src/Consolonia.Core/Controls/MessageBox.axaml.cs(48,43)


[warning] 51-51:
"[UnusedParameter.Local] Parameter 's2' is never used" on /home/runner/work/Consolonia/Consolonia/src/Consolonia.Core/Controls/MessageBox.axaml.cs(51,55)


[warning] 53-53:
"[UnusedParameter.Local] Parameter 's2' is never used" on /home/runner/work/Consolonia/Consolonia/src/Consolonia.Core/Controls/MessageBox.axaml.cs(53,56)

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (2)
src/Consolonia.Core/Controls/MessageBox.axaml.cs (2)

153-153: Remove the partial keyword from MessageBoxViewModel if not required

The MessageBoxViewModel class is declared as an internal partial class. If there are no other partial definitions of this class elsewhere in the codebase, the partial keyword is unnecessary and can be removed for clarity.


11-27: Specify a more precise type for button content properties

The properties Ok, Cancel, Yes, and No are currently of type object. If these properties are intended to hold specific types of content, such as string or a specific control type, consider specifying a more precise type to improve type safety and readability.

Also applies to: 67-89

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 99393d3 and 088be14.

📒 Files selected for processing (1)
  • src/Consolonia.Core/Controls/MessageBox.axaml.cs (1 hunks)
🔇 Additional comments (2)
src/Consolonia.Core/Controls/MessageBox.axaml.cs (2)

46-58: Consider moving focus setting logic to the Loaded event

Currently, the focus is set using the AttachedToVisualTree event handlers for each button within the AttachedToVisualTree event of the MessageBox. This approach might be unnecessarily complex and could lead to timing issues.

Consider moving the focus-setting logic to the Loaded event handler of the MessageBox. This ensures that all visual elements are fully loaded before setting the focus.


171-174: Ensure _text and _title handle null values safely

The fields _text and _title in MessageBoxViewModel are of type string but may receive null values from the constructor parameters text and title. This could lead to a NullReferenceException if null values are passed.

Consider declaring these fields as string? or adding null checks to handle potential null values safely.

Also applies to: 178-207

@jinek
Copy link
Owner

jinek commented Nov 29, 2024

Proposal regarding CA2007 to disable via editorconfig #155

* CA2007 to editorconfig

* NoWant for AVA3001

* Automated JetBrains cleanup

Co-authored-by:  <[email protected]>

---------

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (2)
Directory.Build.props (2)

12-12: Consider a more targeted warning suppression approach.

Global warning suppression in Directory.Build.props affects all projects in the solution. Consider:

  1. Moving this to specific project files where the warning is relevant
  2. Using #pragma warning disable in specific code files
  3. Documenting why this warning needs to be suppressed

This helps maintain better visibility of potential issues and keeps warning suppression scoped to where it's needed.


Line range hint 3-3: Track the TODO comment about mono support.

The TODO comment about adding mono support should be tracked to ensure it's not forgotten.

Would you like me to create a GitHub issue to track the mono support implementation?

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 088be14 and 824defb.

📒 Files selected for processing (7)
  • Directory.Build.props (1 hunks)
  • src/.editorconfig (1 hunks)
  • src/Consolonia.Core/Consolonia.Core.csproj (1 hunks)
  • src/Consolonia.GuiCS/Consolonia.GuiCS.csproj (1 hunks)
  • src/Consolonia.Themes.TurboVision/Consolonia.Themes.TurboVision.csproj (1 hunks)
  • src/Tests/Consolonia.Core.Tests/Consolonia.Core.Tests.csproj (1 hunks)
  • src/Tests/Consolonia.Core.Tests/StorageTests.cs (1 hunks)
✅ Files skipped from review due to trivial changes (2)
  • src/.editorconfig
  • src/Consolonia.GuiCS/Consolonia.GuiCS.csproj
🚧 Files skipped from review as they are similar to previous changes (4)
  • src/Consolonia.Core/Consolonia.Core.csproj
  • src/Consolonia.Themes.TurboVision/Consolonia.Themes.TurboVision.csproj
  • src/Tests/Consolonia.Core.Tests/Consolonia.Core.Tests.csproj
  • src/Tests/Consolonia.Core.Tests/StorageTests.cs

@tomlm tomlm merged commit ad9bc55 into main Nov 30, 2024
2 checks passed
@tomlm tomlm deleted the tomlm/storageProvider branch November 30, 2024 12:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants